Should we consider changing the global functions in node compat mode?

No, that would end up being a breaking change. I can foresee possibly adding a separate compat flag to do so but the ship has already sailed...

That said, we could probably make it so that globalThis.clearTimeout(nodeTimer.setTimeout()) (and vis versa) would work without too much difficulty

Should we consider changing the global functions in node compat mode?

No, that would end up being a breaking change. I can foresee possibly adding a separate compat flag to do so but the ship has already sailed...

That said, we could probably make it so that globalThis.clearTimeout(nodeTimer.setTimeout()) (and vis versa) would work without too much difficulty

In node we can do:

const timeout = setTimeout(...);
timeout.refresh(); // crashes, timeout is a number

We can not do it in worked (both before or after this PR).

One thing that we can do in workerd after this PR is:

import nodeTimers from `node:timers`;
const timeout = nodeTimers.setTimeout(...);
timeout.refresh();

Meaning globalThis.setTimeout is not the same as nodeTimers.setTimeout which I don't think is the right thing to do.

I can foresee possibly adding a separate compat flag to do so but the ship has already sailed...

I don't agree here. I consider this a bug fix that we have to do. That is I think globalThis.setTimeout must return an instance of Timeout as Node does and not a number as it does currently - and I agree there should be a flag.

I think there is no rush to merge this PR at least before we discuss an agree on what the behavior should be.

I think the behavior introduced in this PR can be implemented in unenv for now. In fact [email protected] already has the changes enabling mysql, there is a pending PR to integrate it into wrangler - so ETA is next Tuesday.

edit:

And I think we could safely say that it is a bug fix instead of breaking change.

Because the only thing this can break is typing, i.e. workerd currently returns a number but users never do any operation on that number. They might only pass that number to clearTimeout or clearInterval. So what they might need to do is to update a type from number to Timeout.

I think we need more discussion before landing this.

@vicb Whether or not changing global setTimeout/setImmediate is not within the scope of this pull-request. We already have most of the node:timers through the polyfill, hence, this pull-request is not changing anything that isn't out there. Since this enables mysql and there are teams/customers waiting for this pull-request to land, I propose postponing the discussion of globals once this PR lands, and proceed with this PR.

Meaning globalThis.setTimeout is not the same as nodeTimers.setTimeout which I don't think is the right thing to do.

These are different APIs. Workers is Web compatible first and we're not going to change the behavior of globalThis.setTimeout(...) to match Node.js. Perhaps we can do so at some point with an explicitly opt-in compatibility flag but by default our globalThis.setTimeout(...) should follow the web platform spec while require('node:timer').setTimeout(...) will follow Node.js' spec. The fact that these have different returns and different behaviors is just something we'll have to accept.

I don't agree here. I consider this a bug fix that we have to do. That is I think globalThis.setTimeout must return an instance of Timeout as Node does and not a number as it does currently - and I agree there should be a flag.

Strong -1 on this. It's not a bug that globalThis.setTimeout(...) conforms to the web platform API specification for setTimeout and it's not a bug that nodeTimer.setTimeout(...) conforms to the Node.js version of setTimeout(...). The fact that these work differently is just something we'll need to put up with.

We would be absolutely no different than Deno with this. In Deno, like the implementation here, has globalThis.setTimeout(...) conforming the web platform spec and the node:timers version conforming to Node.js.

@anonrig you said:

Since this enables mysql and there are teams/customers waiting for this pull-request to land, I propose postponing the discussion of globals once this PR lands, and proceed with this PR.

What I said is:

I think the behavior introduced in this PR can be implemented in unenv for now. In fact [email protected] already has the changes enabling mysql, there is a cloudflare/workers-sdk#7806 to integrate it into wrangler - so ETA is next Tuesday.

So my understanding is that "teams/customers waiting for this pull-request to land" will be satisfied with the unenv update alone, but please correct me if I am wrong.

@vicb ... I acknowledge that the situation is unfortunate. Node.js should never have implemented this API in a non-web compatible way, but it did. This is not a reason to block and this is blocking progress on other things. I'm going to dismiss your block so this can proceed.

Nice one. Ship it!